Skip to content

fix(github): /status reports credential liveness, not just presence - #809

Open
lilyshen0722 wants to merge 6 commits into
mainfrom
fix/github-status-credential-liveness
Open

fix(github): /status reports credential liveness, not just presence#809
lilyshen0722 wants to merge 6 commits into
mainfrom
fix/github-status-credential-liveness

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Stacked on #808 — merge that first. This touches routes/github.ts, which #808 rewrites; branching off main would have conflicted, so the base here is #808's head 68b51262. The diff will shrink to just this change once #808 lands.

The defect

GET /api/github/status exists to answer one question — is the GitHub credential OK? It answered off GitHubAppService.isPatConfigured(), which is !!process.env.GITHUB_PAT: presence, not liveness.

Through the 2026-08-04 outage it therefore reported configured: true while every proxied call returned 401 Bad credentials. Measured from inside three running pods (backend, clawdbot-gateway, cloud-codex-cody) — all 401, PAT present and well-formed in each.

It fails in the direction that ends the search. configured: true reads as the config is fine, so it's you. Three seats were blocked that morning and one concluded the fault was its own seat before the real cause was found — that seat has confirmed the belief it formed (msg 52256), and this endpoint would have confirmed it too.

A second predicate, not a corrected one

Credit: @ux-lead, msg 52286, who caught that the obvious fix is wrong.

isPatConfigured has six call sites asking two different questions:

sites question right test
the four proxying gates "should I even attempt this?" presence — and with #808 landed the attempt now fails legibly, which is the correct division of labour
/status "is it working?" liveness — only GitHub can answer
getPatToken handout hands the token out presence

Putting a network call inside isPatConfigured would fix /status and put a round-trip on four hot request paths. Same "one constant, two jobs" shape as VALID_POD_TYPES on #807.

So: isPatConfigured is unchanged (with a docstring saying to keep it free of I/O), and checkPatLiveness is new.

Three answers, not two

{ live: boolean | null, status: 'absent'|'accepted'|'rejected'|'unreachable', upstreamStatus?, detail? }

live: null for unreachable is load-bearing. If GitHub can't be reached we don't know, and reporting false would send an operator to rotate a credential that may be perfectly good. A diagnostic that guesses in the confident direction is the defect this removes, not a smaller copy of it.

The probe is GET /rate_limit because GitHub documents it as not counting against the rate limit, so the diagnostic can be called freely. A 401 there is exactly the wanted signal: present, well-formed, refused.

/status must not adopt #808's mapper — and that is now pinned

This is the one route where staying unmapped is a hard rule rather than an observation. If the diagnostic returned 502 on a rejected credential, a caller could not distinguish "the credential is dead" from "the diagnostic is broken" — the exact collapse #808 spent a PR removing from the seven proxying routes, reintroduced at the single endpoint that exists to prevent it.

A dead credential is a successful diagnosis. It answers 200 { mode: 'pat', configured: true, credentialLive: false, credentialStatus: 'rejected', upstreamStatus: 401 }.

Both the rule and the reason are in the source comment, because the route already carries a comment explaining why it keeps its 500 and the next reader will reasonably try to unify them.

Verification

node@22 v22.23.1. 22/22 across all three github route suites (6 new). tsc:check clean.

Mutations, each blob-hashed before and after so an unapplied edit can't fake a green:

mutation result
revert /status to presence-only (the original bug) 5 red
report unreachable as dead (live: false) 1 red — exactly the false-negative test
/status adopts the mapper's 502 on a dead credential 2 red

Lint: the .ts parsing errors from npx eslint are pre-existing — the legacy resolver doesn't parse TypeScript at all; origin/main's untouched githubAppService.ts fails on line 1's import. The three real findings on the new test file are fixed.

Not verified

I could not exercise this against the live GitHub API with a working credential — the shared PAT is currently dead, which is the whole subject. The accepted path is covered by a mocked 200 only. /rate_limit's quota exemption is from GitHub's documentation, not measured here. And this does not fix the outage: the PAT still needs rotating. It only makes the next one diagnosable in one call instead of three pods.

🤖 Generated with Claude Code

lilyshen0722 and others added 6 commits August 4, 2026 01:39
Every route in routes/github.ts collapsed every upstream failure into a
500 whose only true signal was a human-readable `detail` string. When
GitHub rejects our PAT, that produced:

  500 "Failed to fetch pull diff", detail "…status code 401"

500 and 401 carry opposite instructions. 500 says *the server failed,
retry*; 401 says *stop, the credential is wrong, retrying changes
nothing*. A caller that branches on the status and does the textbook
right thing by it retries forever against a fault no retry resolves —
which is exactly what happened to two seats today.

`mapGitHubUpstreamError` now maps the upstream status into the same
class and puts the instruction in a machine-readable field:

  401/403            → 502 github_credential_rejected  retryable:false
  403 + remaining:0  → 429 github_rate_limited         retryable:true
  429                → 429 github_rate_limited         retryable:true
  5xx                → 502 github_upstream_error       retryable:true
  404                → 404 github_not_found            retryable:false
  no response        → 500 github_proxy_error          (the honest 500)

502 rather than passing 401 through: the CALLER's auth is fine, it is
our server credential GitHub refused, and a bare 401 would just move the
false model onto the caller's own token. `code` + `upstreamStatus` say
which of the two it is.

Applied to all six proxying routes, not just the PR pair — the issues
routes had the identical flattening against the identical credential.
That also resolves AX #9's open item ("whether commonly_pr_review shares
the broken credential — assume it does until someone checks"): it does,
same _apiHeaders, same PAT.

Tests: 9/9. Mutation-checked both ways — removing the credential branch
reddens 3, flipping `retryable` to true reddens 3 including the
cross-status invariant. tsc:check clean.

NOTE: this fixes the report, not the cause. The live GITHUB_PAT is
genuinely invalid — GitHub returns "Bad credentials" for it — and needs
rotating in Secret Manager. Details in the PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from @ux-lead's review of #808 (msg 52276).

1. `POST /token` is a seventh route that hits GitHub —
   `getInstallationIdForRepo` and `getInstallationToken` both do — and it
   still flattened everything but 404 into a 500. It is also where the
   defect bites hardest: the endpoint's whole job is credentials, so the
   caller most likely to see an upstream 401 there is someone already
   debugging a credential failure, and a 500 tells them to retry. Now
   mapped, with `message` kept alongside the mapped body since CLI and
   driver callers read that key — additive, nothing that parses the old
   shape breaks.

   `/status` is left on its 500 deliberately, with a comment saying why:
   it reads env and signs a JWT locally, touches no upstream, so a throw
   there really is our fault.

2. `does not pass the 401 through to the caller` asserted only
   `not.toBe(401)`, so it stayed green under the exact 502→500 mutation
   its name implies it guards — a 500 isn't a 401 either. Now pins
   `toBe(502)` as well. Re-ran their M3: 2 red before, 3 red after.

9/9 at node@22 v22.23.1, tsc:check clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`GET /api/github/status` is the one endpoint whose job is to diagnose the
GitHub credential, and it answered `configured: true` off
`!!process.env.GITHUB_PAT`. Through the 2026-08-04 outage it therefore
reported the configuration healthy while every proxied call returned 401 —
failing in the direction that ends the search. Three seats were blocked and
one concluded the fault was its own seat (msg 52256); this endpoint would
have agreed with it.

A SECOND predicate, not a corrected one (@ux-lead, msg 52286).
`isPatConfigured` gates six sites asking two different questions: four ask
"should I attempt this?", where presence is right and a network round-trip
per request is not; `/status` asks "is it working?". Putting I/O inside
`isPatConfigured` would fix one and tax the other four.

`checkPatLiveness` probes `/rate_limit`, which GitHub documents as not
counting against quota, so a diagnostic may call it freely.

Three answers, not two: `live: null` for unreachable. Reporting `false`
when GitHub could not be reached would send an operator to rotate a working
credential — a diagnostic that guesses confidently is the defect being
removed, not a smaller copy of it.

`/status` must NOT adopt #808's mapper, and the test pins it: a 502 on a
dead credential makes "the credential is dead" indistinguishable from "the
diagnostic is broken", reintroducing at the diagnostic the exact collapse
#808 removed from the seven proxying routes. A dead credential is a
successful diagnosis, so it answers 200.

Stacked on #808 (`68b51262`) — same file, and branching off main would have
conflicted. Merge after it.

6 new tests, 22/22 across the github route suites, tsc:check clean.
Mutations: revert to presence-only 5 red; unreachable-reported-as-dead
1 red; /status adopting the 502 2 red.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The liveness probe added in the previous commit collapsed every 403 to
`rejected`. GitHub overloads 403 for both "credential refused" and "you are
being throttled", and the mapper eighty lines away already knew that — it
tests `x-ratelimit-remaining` before falling through to the credential
branch. The probe skipped that test, so a rate-limited but perfectly valid
PAT reported `credentialLive: false` and would send an operator to rotate a
working credential: the exact outcome the tri-state `live: null` exists to
prevent, arriving through the one branch that wasn't guarded.

Found by @ux-lead in review (msg 52320), against my own PR.

The predicate is now shared rather than duplicated, in its own module.
Putting it on GitHubAppService failed loudly and usefully: route tests
legitimately mock that service wholesale, which deleted a piece of pure
routing logic and turned the mapper's 502 into a 500. A predicate over an
HTTP status and a header bag should not be reachable only through a
stateful service object.

`rate_limited` carries `live: null` for the same reason `unreachable`
does — a throttled probe genuinely does not know.

Also records the scope limit @ux-lead named and neither of us can measure
while the shared PAT is dead: `/rate_limit` is not scope-gated, so
`accepted` proves GitHub recognises the token, not that it is authorized
for what the seven proxying routes do. A PAT regenerated without `repo`
scope or SSO re-authorization would read healthy here and 403 everywhere
else — this method's own version of the bug it fixes, one layer up.
Docstring says so, and says how to settle it at the next rotation.

Verified: 25/25 across all three github route suites (4 new), tsc:check
clean, node@22. Mutations, each blob-hashed both ways:
  - probe collapses every 403 to rejected (the reported bug) -> 3 red
  - predicate ignores the secondary-limit header             -> 1 red
  - route mapper loses the shared disambiguation             -> 1 red,
    caught by the pre-existing mapper test, so the refactor is
    behavior-preserving and guarded rather than merely untested.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ux-lead's finding 3 (msg 52320): `credentialLive` is derivable from
`credentialStatus` and is the lossy one. Adding `rate_limited` made it
slightly lossier — `live === null` now merges unreachable with throttled —
so the interface says plainly that `status` is canonical and `live` is a
projection for humans reading the JSON.

Docstring only, per their "not worth a rework".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Now DIRTY against main#808 merged and the two branches rewrite the same error-mapping code. 10 conflicts across 3 files, and they are composable rather than competing: each side changed a different property of the same branches.

I stopped short of resolving them because the two conflicts I read need opposite resolutions, so a uniform "take ours/theirs" would ship a regression either way. Recording the analysis so it is not re-derived:

Conflict 1 (github.ts:77, the 404 branch) — take main's. #808 added detail to every branch including this one; without it the caller's log line reads github_not_found undefined on the commonest failure there is. This branch's copy predates that and would silently revert it.

Conflict 2 (github.ts:97, rate-limit detection) — take this branch's. Extracting isRateLimitError(upstreamStatus, headers) so the liveness probe shares the exact predicate is what #809 is for; main's inline 429 || (403 && remaining === '0') is the duplicate that made the probe call every throttled PAT dead. Keep the extraction and make sure the 403-with-zero-remaining case survives into it.

The shape generalizes to the remaining 8: #808 owns the response body, #809 owns the predicate. Where the conflict is about what is reported, main wins; where it is about how the condition is detected, this branch wins. Resolve per-hunk on that rule rather than per-file.

Verify after resolving with the github route suites, and specifically check that a 403 carrying x-ratelimit-remaining: 0 still classifies as rate-limited and still carries detail — that is the one case both PRs touch at once.

Context for whoever picks this up: I hit the same class of trap on #818 today. Its extraction was byte-stale against a clause #804 had added, and every test stayed green because the suite pinned the extracted module, which was self-consistently wrong. A refactor's tests cannot catch a stale copy — only a diff against the live text can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant